Track SessionContext instead of InvocationContext #4916

andrewyuq · 2024-09-25T22:32:57Z

Since the shift from displaying one trigger at a time to displaying multiple triggers at a time, use SessionContext as the disposable for the session states.
Introduce the concept of "display session" which can show multiple triggers at a time and sessionContext is to hold states for that session.
ongoingRequests will be a map of <jobId, InvocationContext> where InvocationContext is the state of each trigger.
Managed these states globally by dynamically adding Trigger states into ongoingRequests and whenever the user made a decision(accept or reject), conclude decisions for all the ongoing triggers and clear ongoingRequest, so that a new display session can start later.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Description

Checklist

My code follows the code style of this project
I have added tests to cover my changes
A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

1. Since the shift from displaying one trigger at a time to displaying multiple triggers at a time, use SessionContext as the disposable for the session states. 2. Introduce the concept of "display session" which can show multiple triggers at a time and sessionContext is to hold states for that session. 3. ongoingRequests will be a map of <jobId, InvocationContext> where InvocationContext is the state of each trigger. 4. Managed these states globally by dynamically adding Trigger states into ongoingRequests and whenever the user made a decision(accept or reject), conclude decisions for all the ongoing triggers and clear ongoingRequest, so that a new display session can start later.

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

rli · 2024-09-25T22:58:12Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

    private var refreshFailure: Int = 0
+    private val ongoingRequests = mutableMapOf<Int, InvocationContext?>()
+    val ongoingRequestsContext = mutableMapOf<Int, RequestContext>()
+    private var jobId = 0


can this overflow?

We have a monthly trigger limit which is 25000, so jobId will increment 25000 each month if they don't restart IDE.

rli · 2024-09-25T23:01:08Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

+    private val ongoingRequests = mutableMapOf<Int, InvocationContext?>()
+    val ongoingRequestsContext = mutableMapOf<Int, RequestContext>()


not sure if this is the best pattern if the exact jobId is only relevant for "which is the last request", which we can determine in other ways

jobId is only to separate different triggers and is a local identifier only, (same as sessionId except we know the jobId as soon as a trigger happens before receiving the response)

rli · 2024-09-25T23:01:45Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

            return
        }
+        val caretContext = requestContext.fileContextInfo.caretContext
+        ongoingRequestsContext.forEach { (k, v) ->


for example, this is guaranteed to iterate in-order from first request inserted in the map to last

yea, the purpose is not just to identify which the last request is -- but more to separate states of different triggers.

Later on when we send UserTriggerDecisions we will need to send one for each jobId(trigger), having a jobId here and logging it keeps track of what happened E2E for this specific trigger.

rli · 2024-09-25T23:02:17Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

        // It's possible and ok that coroutine will keep running until the next time we check it's state.
        // As long as we don't show to the user extra info we are good.
-        val coroutineScope = disposableCoroutineScope(popup)
+        val coroutineScope = projectCoroutineScope(requestContext.project)


CodeWhispererService has a service-level scope that should be used instead

rli · 2024-09-25T23:22:24Z

...unity/src/software/aws/toolkits/jetbrains/services/codewhisperer/model/CodeWhispererModel.kt


 data class RecommendationContext(
-    val details: List<DetailContext>,
+    val details: MutableList<DetailContext>,


not seeing why need mutable

Now each trigger will only have one state InvocationContext and is stored in ongoingRequests, when subsequent suggestions sent back to client, they will be added to this mutable details, and other parts of InvocationContext remains the same.

private fun updateStates( states: InvocationContext, response: GenerateCompletionsResponse ): InvocationContext { val recommendationContext = states.recommendationContext val newDetailContexts = CodeWhispererRecommendationManager.getInstance().buildDetailContext( states.requestContext, recommendationContext.userInputSinceInvocation, response.completions(), response.responseMetadata().requestId() ) recommendationContext.details.addAll(newDetailContexts) return states }

otherwise we will have to replace InvocationContext which I feel is a bit unnecessary.

rli · 2024-09-25T23:25:05Z

...unity/src/software/aws/toolkits/jetbrains/services/codewhisperer/model/CodeWhispererModel.kt

+        if (hasAccepted) {
+            popup?.closeOk(null)
+        } else {
+            popup?.cancel()
+        }
+        popup?.let { Disposer.dispose(it) }
+        popup = null


popup manager and sessioncontext are so tied together that it feels like it should just be handled together

popup now becomes a state of a session (1 session 1 popup) so I put it as a field of SessionContext, and then I let disposing SessionContext handles everything so I don't have to call

CWService.getInstance().disposeDisplaySession() popup.cancel()/close()

separately.

rli · 2024-09-25T23:26:08Z

...rc/software/aws/toolkits/jetbrains/services/codewhisperer/popup/CodeWhispererPopupManager.kt

-            it.closeOk(null)
-            Disposer.dispose(it)
+    fun showPopup(sessionContext: SessionContext, force: Boolean = false) {
+        val p = sessionContext.editor.offsetToXY(sessionContext.popupDisplayOffset)


popupOffset?

rli · 2024-09-25T23:35:53Z

...rc/software/aws/toolkits/jetbrains/services/codewhisperer/popup/CodeWhispererPopupManager.kt

+    private fun getValidCount(): Int =
+        CodeWhispererService.getInstance().getAllSuggestionsPreviewInfo().filter { isValidRecommendation(it) }.size
+
+    private fun getValidSelectedIndex(selectedIndex: Int): Int {


feels like this could be cleaner but i see that it's the same as the old logic

Will-ShaoHua · 2024-10-03T18:03:41Z

can we add some doc strings for different models

Will-ShaoHua · 2024-10-03T18:05:08Z

...unity/src/software/aws/toolkits/jetbrains/services/codewhisperer/model/CodeWhispererModel.kt

+    val jobId: Int,
+    val detail: DetailContext,
+    val userInput: String,
+    val typeahead: String,


dumb q, what's the difference between these 2 typeaheads

previously we have typeahead and typeaheadoriginal for the purpose of accounting leading spaces in the typeahead,
for example, typeaheadoriginal will be System and typeahead will be System, and then we use typeahead to do the suggestion matching logic. This is to preserve the suggestions as much as possible.

Now we trigger more so we have more display oppotunities, I feel like this becomes a bit unnecessary and makes the logic complicated, so remove typeahead original to make it no longer accomodate leading spaces.

Will-ShaoHua · 2024-10-03T18:10:40Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

+    private val ongoingRequests = mutableMapOf<Int, InvocationContext?>()
+    val ongoingRequestsContext = mutableMapOf<Int, RequestContext>()
+    private var jobId = 0
+    private var sessionContext: SessionContext? = null


not sure do we need this to be thread safe

Technically it needs, so the current rule is to modify this only in EDT so I believe in various places I have @RequiresEDT annotation including disposeDisplaySession, and initialing SessionContext also is in EDT.

Will-ShaoHua · 2024-10-03T19:42:34Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

 class CodeWhispererService(private val cs: CoroutineScope) : Disposable {
    private val codeInsightSettingsFacade = CodeInsightsSettingsFacade()
    private var refreshFailure: Int = 0
+    private val ongoingRequests = mutableMapOf<Int, InvocationContext?>()


what's the case when InvocationContext being null

before the InvocationContext for the specific jobId is added, it can be null, we did some null check to know if it's first request/response.

Will-ShaoHua · 2024-10-03T19:47:49Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

+                                            processCodeWhispererUI(
+                                                sessionContext,
+                                                it,
+                                                ongoingRequests[currentJobId],
+                                                cs,
+                                                currentJobId
+                                            )
+                                            if (!ongoingRequests.contains(currentJobId)) {
+                                                cs.coroutineContext.cancel()
+                                            }


andrew could you help me understand why isn't this in the format of

if (ongoingRequests.contains(currentJobId)) { processCodeWhispererUi() }

but execute processCodeWhispererUi and cancel the coroutine later, what's the expected behavior here?

so intially there's no InvocationContext in ongoingRequests, and then we contruct it in initState in processCodeWhispererUi and put it into the map. If after this function it's still null, then it means state creation failed due to e.g. all discarded suggestions or all empty suggestions.

Will-ShaoHua · 2024-10-03T19:48:47Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

+                                    ongoingRequests[currentJobId],
+                                    cs,
+                                    currentJobId
+                                )


why here doesn't need the

if (!ongoingRequests.contains(currentJobId)) { cs.coroutineContext.cancel() }

check

I think it still needs, I will add it in the following PR.

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

Will-ShaoHua · 2024-10-03T20:04:55Z

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt

                    sessionId = e.awsErrorDetails().sdkHttpResponse().headers().getOrDefault(KET_SESSION_ID, listOf(requestId))[0]
                    displayMessage = e.awsErrorDetails().errorMessage() ?: message("codewhisperer.trigger.error.server_side")
-                } else if (e is software.amazon.awssdk.services.codewhispererruntime.model.CodeWhispererRuntimeException) {
+                } else if (e is CodeWhispererRuntimeException) {


thanks for the change!

...tware/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererInvocationStatus.kt

rli · 2024-10-03T20:36:27Z

...unity/src/software/aws/toolkits/jetbrains/services/codewhisperer/model/CodeWhispererModel.kt

+    val project: Project,
+    val editor: Editor,
+    var popup: JBPopup? = null,
+    var selectedIndex: Int = -1,
    val seen: MutableSet<Int> = mutableSetOf(),
-    val isFirstTimeShowingPopup: Boolean = true,
+    var isFirstTimeShowingPopup: Boolean = true,
    var toBeRemovedHighlighter: RangeHighlighter? = null,
-    var insertEndOffset: Int = -1
-)
+    var insertEndOffset: Int = -1,
+    var popupOffset: Int = -1,
+    val latencyContext: LatencyContext,
+    var hasAccepted: Boolean = false
+) : Disposable {


some of these can be private / don't need to be initialized in the primary constructor since the initial value is always expected to be the same

…try-2

andrewyuq requested a review from rli September 25, 2024 22:32

andrewyuq requested a review from a team as a code owner September 25, 2024 22:32

rli reviewed Sep 25, 2024

View reviewed changes

address comments

d37fec2

Will-ShaoHua reviewed Oct 3, 2024

View reviewed changes

...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt Show resolved Hide resolved

Will-ShaoHua reviewed Oct 3, 2024

View reviewed changes

...tware/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererInvocationStatus.kt Show resolved Hide resolved

Will-ShaoHua self-requested a review October 3, 2024 20:27

Will-ShaoHua approved these changes Oct 3, 2024

View reviewed changes

rli approved these changes Oct 3, 2024

View reviewed changes

andrewyuq changed the base branch from main to auto-trigger-revamp October 3, 2024 23:18

Merge remote-tracking branch 'aws/auto-trigger-revamp' into cw-teleme…

156159e

…try-2

andrewyuq merged commit af3c9e6 into aws:auto-trigger-revamp Oct 4, 2024
0 of 8 checks passed

		private val ongoingRequests = mutableMapOf<Int, InvocationContext?>()
		val ongoingRequestsContext = mutableMapOf<Int, RequestContext>()

Track SessionContext instead of InvocationContext #4916

Track SessionContext instead of InvocationContext #4916

Uh oh!

Conversation

andrewyuq commented Sep 25, 2024

Types of changes

Description

Checklist

License

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewyuq Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewyuq Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewyuq Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Will-ShaoHua commented Oct 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

andrewyuq Sep 26, 2024 •

edited

Loading

andrewyuq Sep 26, 2024 •

edited

Loading

andrewyuq Sep 26, 2024 •

edited

Loading